Read only when the channel does not auto-read - #2302
Conversation
AsyncHttpClientHandler requested a read from both channelActive and channelReadComplete. Netty's HeadContext already calls Channel#read() right after firing either event whenever autoRead is on, so every read cycle traversed the outbound pipeline and reached doBeginRead twice instead of once. AsyncHttpClient never clears autoRead and Netty defaults it to on, so that was the normal path for every connection, and the waste grew with the number of read cycles a response took. Drive the read from here only when autoRead is off. That keeps working for a caller who disables autoRead through a channel option, which is also a fix in its own right: such a caller previously had the setting silently defeated by these two unconditional reads. Verified against netty 4.2.16.Final rather than assumed. HeadContext calls readIfIsAutoRead() from both channelActive and channelReadComplete, DefaultChannelConfig initialises autoRead to on, and HTTP/2 stream channels share both behaviours: their pipeline extends DefaultChannelPipeline and Http2StreamChannelConfig extends DefaultChannelConfig without overriding isAutoRead. The change therefore holds for all three subclasses, HTTP/1.1, WebSocket and HTTP/2. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| @Override | ||
| public void channelActive(ChannelHandlerContext ctx) { | ||
| ctx.read(); | ||
| readIfNotAutoRead(ctx); |
There was a problem hiding this comment.
Worth noting for anyone else reading this, HeadContext calls readIfIsAutoRead after fireChannelActive returns, not as part of the propagation. So it does not matter that this handler never forwards the event, the read still happens. Same for channelReadComplete below.
There was a problem hiding this comment.
Right, and that ordering is what makes this safe. The javadoc says it now, so the next reader does not have to wonder whether the handler swallowing the event matters.
| @Override | ||
| public void channelReadComplete(ChannelHandlerContext ctx) { | ||
| ctx.read(); | ||
| readIfNotAutoRead(ctx); |
There was a problem hiding this comment.
I traced the HTTP/2 side since that is where dropping one of two reads could have bitten us. Inside the read loop the second read only flipped readStatus from IN_PROGRESS to REQUESTED and resetReadStatus put it straight back, so the stream channel ends in the same state either way. Outside the loop the status is already reset to IDLE before the event is fired, so the HeadContext read does the full drain on its own. No read is lost, and we lose a layer of re-entrancy.
There was a problem hiding this comment.
Thank you for tracing that. The pipeline and config side I could confirm from the bytecode, but the readStatus transitions inside the read loop were the part I could not rule out that way.
| * Requests the next read only when the channel will not do it by itself. Netty's HeadContext already | ||
| * calls Channel#read() after firing channelActive and channelReadComplete whenever autoRead is on, so | ||
| * reading here as well only repeated the outbound pipeline traversal and doBeginRead. AsyncHttpClient | ||
| * never clears autoRead, which defaults to on, so this is the usual path; a caller that turns it off |
There was a problem hiding this comment.
This sentence is a claim about the rest of the codebase and it goes stale the day someone puts AUTO_READ in a config. Can we keep the comment to the mechanism and drop the survey of current usage.
There was a problem hiding this comment.
Dropped. Fair point that it dates the comment for no benefit; the mechanism holds whether or not anything in the codebase sets AUTO_READ.
| * through a channel option still needs reads to be driven from here, which is why the call is kept | ||
| * rather than dropped. | ||
| */ | ||
| private static void readIfNotAutoRead(ChannelHandlerContext ctx) { |
There was a problem hiding this comment.
Netty spells this readIfNeeded in SslHandler, and Http2ConnectionHandler.channelReadComplete0 does the same guard inline. Matching the name and pointing at one of those in the javadoc would make it obvious this is the house style and not something we invented.
There was a problem hiding this comment.
Renamed to readIfNeeded, and the javadoc now points at SslHandler and at the inline version in Http2ConnectionHandler.channelReadComplete0. Better to match what Netty already does in two places than to invent a name for it.
| * rather than dropped. | ||
| */ | ||
| private static void readIfNotAutoRead(ChannelHandlerContext ctx) { | ||
| if (!ctx.channel().config().isAutoRead()) { |
There was a problem hiding this comment.
Right call checking this per event instead of caching it at channelActive. autoRead is a volatile field and a user handler can flip it mid connection, so a cached value would go wrong quietly.
There was a problem hiding this comment.
Yes. autoRead is volatile and a user handler can flip it mid connection, so a value cached at channelActive would have been wrong in a way that produces a stalled connection rather than a failure.
Review feedback on AsyncHttpClient#2302. Netty spells this guard readIfNeeded in SslHandler and inlines the same test in Http2ConnectionHandler.channelReadComplete0, so take that name and point at both. It makes clear the shape is Netty's rather than something invented here. Drop the sentence stating that AsyncHttpClient never clears autoRead. It is a survey of current usage rather than of the mechanism, and it goes stale the day a config grows an AUTO_READ option. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Problem
AsyncHttpClientHandlerrequested a read from both lifecycle callbacks:Netty's
HeadContextalready callsChannel#read()immediately after firingeither event whenever
autoReadis on. AsyncHttpClient never clearsautoRead(no
ChannelOption.AUTO_READanywhere in the codebase) and Netty defaults it toon, so that was the path for every connection: each read cycle traversed the
outbound pipeline and reached
doBeginReadtwice instead of once. The cost isper read cycle, so it scales with how many reads a response takes.
Change
Drive the read from the handler only when
autoReadis off.That is also a fix in its own right: a caller who disabled
autoReadthroughsetChannelOptionpreviously had the setting silently defeated by these twounconditional reads, since the handler kept requesting reads regardless.
Verification of the Netty behaviour
Checked against
netty-codec-http2/netty-transport4.2.16.Final, theversion this project builds against, rather than assumed:
DefaultChannelPipeline$HeadContext.channelActiveand.channelReadCompleteboth call
readIfIsAutoRead(), which isif (channel.config().isAutoRead()) channel.read();DefaultChannelConfig's constructor initialisesautoReadto1, so on.AbstractHttp2StreamChannel$2 extends DefaultChannelPipeline, andHttp2StreamChannelConfig extends DefaultChannelConfigwithout overridingisAutoRead.The last point matters because this is the shared base class of
HttpHandler,WebSocketHandlerandHttp2Handler, and neither of the two callbacks isoverridden by any of them, so the change applies to HTTP/1.1, WebSocket and
HTTP/2 stream channels alike.
Verification of the build
mvnw clean verify- BUILD SUCCESS, 1371 tests, 0 failures, 0 errors,19 skipped. Error Prone, NullAway and Revapi all clean.
The suites covering the paths most exposed to a change in read behaviour are
green: 168 HTTP/2 tests (
BasicHttp2Test,Http2MultiplexBugRegressionTest,Http2StreamingBodyFlowControlTest,Http2StreamOrphanRegressionTest,Http2ConformanceRegressionTestand the rest) and 36 WebSocket tests(
TextMessageTest,ByteMessageTest,CloseCodeReasonMessageTest,WebSocketWriteFutureTest,ws.ProxyTunnellingTest).Caveat on the testing gate:
AGENTS.mdrequires the build to run on JDK 11 andno JDK 11 is installed on this machine, so it was run on JDK 17 (also in the
CI matrix). The JDK 11 leg of CI on this PR is the real gate.
No public API change. Same review pass as #2300 and #2301.
Claude Code on behalf of @pavel-ptashyts
🤖 Generated with Claude Code